-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable RISCV #52787
Enable RISCV #52787
Conversation
dvc94ch
commented
Jul 27, 2018
- Enable LLVM backend.
- Implement call abi.
- Add built-in target riscv32imac-unknown-none.
- Enable CI.
src/librustc_target/spec/mod.rs
Outdated
@@ -373,6 +373,8 @@ supported_targets! { | |||
("armv7-unknown-cloudabi-eabihf", armv7_unknown_cloudabi_eabihf), | |||
("i686-unknown-cloudabi", i686_unknown_cloudabi), | |||
("x86_64-unknown-cloudabi", x86_64_unknown_cloudabi), | |||
|
|||
("riscv32imac-unknown-none", riscv32imac_unknown_none), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the last part of the triple should be unknown-elf
to match the triple used by the GCC tooling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is necessary. If it where something other than elf we'd have riscv32imac-unknown-(darwin|msvc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll be adding riscv64imac-linux-gnu
in the future. I'd like to see some consistency in the triple format: $ARCH_PLUS_INSN_SET-$OS-$ENV_OR_FORMAT
. *-unknown-none
sounds like either the environment or the object format is none
and that's not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the unknown
refers to the cpu vendor, in case there are vendor specific bugs/optimizations/extensions. So is riscv32imac-unknown-none-elf
ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like the most future proof choice. 👍
We may want to bikeshed the name of the target before landing this. So far most (all?) the built-in targets use I should also note that the RV32IMAC instruction set is the instruction set implemented in the SiFive Freedom E310 (the only RISCV silicon implementation I'm aware of (*)), and that @dvc94ch has been testing this target with that hardware for quite some time now. (*) There's also the Freedom U540, a 64-bit multi-core RISCV SoC, but I don't know if anyone has one yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, but I'd like to see the last part of the target name to be changed to be more similar to the prefix used by the GCC toolchain (riscv32-unknown-elf / riscv64-unknown-elf).
I'll defer the decision of enabling the production of rust-std components right now and doing the final sign off to @alexcrichton.
Nice! To enable RISCV by default in LLVM I think that these lines will also want to change |
For CI purposes, some standard questions we like to ask are:
Other than that I'd just have the same naming questions @japaric is bringin up! |
To clarify the target this PR is adding is a
It's an experimental backend like the WebAssembly backend. I think it has been in development for a longer time though (could be wrong). |
Ah sorry I missed that! The question is similar though in the sense that the backend can handle all of libcore? It's true that WebAssembly is classified as "experimental" but it's been quite solid for our use cases at least, never regressing in something we added to libcore, only discovering bugs after the fact :) |
Should be around or grow to the size of the mips backend.
The people working on the riscv llvm backend are diligent and knowledgeable. Since there were enough features to compile libcore I have rarely hit any asserts. I had more problems with debug info or missing features required for embedded development (unimplemented assembler directives and such). Debug info worked out of the box with the llvm from nightly, although I have not used it extensively yet. Since the c extension actually works now I'm getting instruction missaligned errors which I haven't fixed yet (in my bsp crates). openocd and gdb like to crash - don't know why. |
Ok great! Sounds like this is basically good to go from that perspective :) |
FWIW, I'm seeing an increase of 2MB (+3.33%) in librustc_codegen_llvm-llvm.so. |
@dvc94ch do you know if this target is supposed to support CAS operations natively? The following program results in an LLVM error: #![allow(warnings)]
#![crate_type = "lib"]
#![no_std]
use core::sync::atomic::{AtomicUsize, Ordering};
#[no_mangle]
pub fn foo(x: &AtomicUsize, y: usize) -> usize {
x.swap(y, Ordering::SeqCst)
} $ rustc -O --emit=obj --target riscv32imac-unknown-none riscv.rs
LLVM ERROR: Cannot select: 0x7f2e4c036840: i32,ch = AtomicSwap<(volatile load store seq_cst 4 on %ir.0)> 0x7f2e4c01f418, 0x7f2e4c036708, 0x7f2e4c0367d8
0x7f2e4c036708: i32,ch = CopyFromReg 0x7f2e4c01f418, Register:i32 %0
0x7f2e4c0366a0: i32 = Register %0
0x7f2e4c0367d8: i32,ch = CopyFromReg 0x7f2e4c01f418, Register:i32 %1
0x7f2e4c036770: i32 = Register %1
In function: foo Is this an LLVM bug? Or is the target supposed to have (Codegen of atomic loads / stores works fine) |
I've seen that before: lowRISC/riscv-llvm#56 atomic support is incomplete. This also prevents porting libstd currently. Interesting to know that the issue hasn't been fixed yet. |
You mean the "+c" feature, right? Do you think the implementation is buggy? Or are you using inline assembly and that could be the cause of the alignment problem? If "+c" is buggy we could remove it for now. It only reduces the size of the binary, right? Unlike "+m", removing it shouldn't increase the number of intrinsics needed (e.g. __mulsi). |
In that case we could set |
I think it's my inline asm. But when I checked the spec it say's "The C extension is compatible with all other standard instruction extensions. The C extension allows 16-bit instructions to be freely intermixed with 32-bit instructions, with the latter now able to start on any 16-bit boundary." So it's not immediately clear what the issue might be. But I haven't had time to investigate it thoroughly yet. EDIT: It works fine without the c extension |
cc @asb riscv llvm maintainer |
Codegen for atomics has been committed about a month ago upstream, so it wasn't in LLVM 7 which we're using. |
Regarding the triple, I wish I could just argue for taking inspiration from the C toolchain conventions. Unfortunately, rustc's situation is sufficiently different -- no analogue to multilib -- that this might be difficult. So we're probably forced to ship a standard library for a specific set of extensions, with no way to
So we're effectively shipping a toolchain tied to a particular set of extensions, rather than really presenting a "base ISA + pick your own extensions" view to the user. For this reason, I think it's fine to put something like |
Seems reasonable to me @japaric! |
I disabled the c extension and atomic_cas. This commit is intended to be reverted in the future. |
In that case moving to the latest LLVM might fix. LLVM upgrades can be ... tricky so I'd suggest landing this now and do the LLVM upgrade in a separate PR.
Pure MIR rlibs will let us tweak codegen (e.g. "+c", "+m") at the top level without changing the target and recompiling the whole dependency graph, but they won't help with features like "+a" which conditionally compile code like AtomicUsize. I don't think we can have a single built-in
Off topic: multilib doesn't look that different to me. $ # these are nameless targets
$ arm-none-eabi-gcc -print-multi-lib | grep v7e
thumb/v7e-m/nofp;@mthumb@march=armv7e-m@mfloat-abi=soft # this one is like our `thumbv7em-none-eabi` target
thumb/v7e-m+fp/softfp;@mthumb@march=armv7e-m+fp@mfloat-abi=softfp
thumb/v7e-m+fp/hard;@mthumb@march=armv7e-m+fp@mfloat-abi=hard
thumb/v7e-m+dp/softfp;@mthumb@march=armv7e-m+fp.dp@mfloat-abi=softfp
thumb/v7e-m+dp/hard;@mthumb@march=armv7e-m+fp.dp@mfloat-abi=hard
@alexcrichton @dvc94ch Alright this LGTM. I'm happy with the name too. |
I think these three patches are also required which haven't been reviewed yet. There is only one RISCV patch that isn't in rust's llvm, so there's no hurry. https://reviews.llvm.org/D48131 |
Yes, in general we'd really need to compile the standard library on the user's machine with codegen options they choose (Xargo-style). Or a multilib-style setup...
At least the way RISC-V GCC does it, users can add any extensions they like to their own compilation process and transparently get a working library that uses either exactly those features or a reasonably large subset of them (so there aren't exponentially many minor variations of the libraries on disk). In contrast, while we can ship a libstd component for every set of features we like, users will always have to pick one of those manually and specify it everywhere. |
@dvc94ch do you want to merge this before the LLVM patches land? It's fine from our perspective to do so I believe and we can always update LLVM later once the patches land. |
@alexcrichton Sounds good to me! |
📌 Commit d974dc9 has been approved by |
⌛ Testing commit d974dc9 with merge 3452276698f71dd589e8f184e0f8f62bfa7f9422... |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors: retry |
Enable RISCV - Enable LLVM backend. - Implement call abi. - Add built-in target riscv32imac-unknown-none. - Enable CI.
☀️ Test successful - status-appveyor, status-travis |
I have one of the Freedom U540 boards -- feel free to ping me or CC me on things if you want me to test something out! |
@zarvox Nice! Atomic support is not complete in the version of LLVM we are using so the AtomicUsize API is not complete and std can't be built. Thus we can't build Linux (std) programs at the moment. When it becomes possible to build Linux programs for RISCV we'll let you know. |
FYI: I probably won't be doing this work since a U540 board costs around 1000$ and I'm more interested in rust on riscv soft cores at the moment. |
Hi, RISC-V LLVM code owner here. Many thanks to @dvc94ch and everyone else who has contributed to enabling RISC-V support in Rust. I'd love to see support for Rust on RISC-V Linux targets too, though it's worth noting there are other pre-requisites beyond the atomics support that isn't fully merged (e.g. hardfloat ABI, PIC, TLS). We should make better use of the upstream LLVM bugzilla to keep track of unimplemented features that are useful to the Rust community (e.g. the .option push/pop mentioned in this thread). |
I am getting this
|
|
after figuring out that I had issues with building 'core' (I have installed rustc via apt, previously) I tried to uninstall rustc (and the rest related packages).
|